Skip to content

Convert Zephyr primitives to a better initialization and use framework #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Mar 14, 2025

Conversation

d3zd3z
Copy link
Collaborator

@d3zd3z d3zd3z commented Feb 20, 2025

The existing object wrappers for Zephyr objects centers around a "Fixed" type, which is a enum, containing either a static reference, or a Pin<Box<T>>. This means that anything not declared with the kernel_obj macros will require allocation.

We can get rid of the need for allocation and also improve performance (very slightly). Instead of this enum which then hides another reference, a ZephyrObject is just a regular struct, with an atomic and an UnsafeCell holding the real Zephyr object. The atomic is used to allow the initialization to happen on first use instead of at constructor time. It also detects if one of these objects has been moved (resulting in a panic), because requiring the use of Pin on these types would make this mechanism even less hygienic than the kernel_obj macros.

The new objects have const constructors, which allows them to be plainly declared as statics, or used in regular code, without requiring an alloc to happen under the hood. The only constraint is that the objects cannot be moved after their first use. For embedded, this is generally not difficult, as the objects will usually be static, put in something like a StaticCell, or allocated in an Rc or Arc.

There is, unfortunately, a bit of a minor API change. Semaphore's currently return a Result, and the Result type is not particularly usable in const. Semaphore::new now directly returns the Semaphore, and if the constructor values are nonsensical (either a zero limit, or an initial value larger than the limit), it will panic instead of returning an error.

@d3zd3z d3zd3z force-pushed the atomic-objects branch 2 times, most recently from b86831b to 29db006 Compare February 24, 2025 23:37
d3zd3z added 10 commits March 7, 2025 12:32
Implement a new wrapping system for Zephyr Objects.  Instead of trying
to either use a static or a pinned-box, use an atomic to detect
initialization, as well as improper moves.

This allows for objects to have `const` constructors, allowing them to
be declared as simple statics and just shared.

Subsequent patches will implement this for various primitives.

Signed-off-by: David Brown <[email protected]>
Move Semaphores to the new `ZephyrObject` system, allowing the
constructor to be const.  Unfortunately, `Result` isn't generally useful
with const, and as such, this changes the API for `Semaphore::new()` to
just return the semaphore instead of a Result.  Instead, if a semaphore
constructor has inconsitent arguments, it will panic.  Given that
semaphore constructors are generally quite simple, this is a minor
issue.

However, it will involve removing a lot of `.unwrap()` or `.expect(...)`
calls on semaphores.

The samples have been changed for the API change, but mostly not changed
to take advantage of the easier ways to use them.  It is generally no
longer necessary to put semaphores in an Arc or Rc, but just to allocate
them in something like a StaticCell, and then pass them around by
reference.

Signed-off-by: David Brown <[email protected]>
Convert to the new atomic initializer format. This makes the `new()`
constructor `const` allowing queues to be used as statics.

Similar to the API change to Semaphore, the constructor no longer
returns a Result, just the Queue directly.

Signed-off-by: David Brown <[email protected]>
Adapt to the API change in both Semaphore and Queue. This still
allocates using Arc, though.

Signed-off-by: David Brown <[email protected]>
Update the sys Mutex/Condvar to use the new object initialization, and
update the uses.  This requires a few fixes:

- The SimpleTls no longer needs the StaticTls helper, and we can just
  use a plain global static Mutex.
- Bounded channels add items to a free queue during init.  To avoid
  moving this queue after items have been inserted, we place it in a
  box.  Given that the data of bounded queues is also boxed, this isn't
  really a concern.

Signed-off-by: David Brown <[email protected]>
This was an experiment to see if using an atomic to manage semaphore
initialization was workable.  Now that the main Semaphore implementation
works this way, this can just be removed.

Signed-off-by: David Brown <[email protected]>
Drop the iterations so not as much time is spent in CI.  This still
gives meaning functional results for CI, and the increased value can be
used for actual benchmarking.

Signed-off-by: David Brown <[email protected]>
Although there is still a lot of allocation left (the crossbeam API for
channels is kind of built around it), convert the Semaphores in the
benchmark to be static.  This reduces overhead a little bit, because the
reference counts of Arc don't need to be maintained, and also
demonstrates how StaticCell can be used to allocate things like arrays
of objects statically.

Signed-off-by: David Brown <[email protected]>
Clean up formatting

Signed-off-by: David Brown <[email protected]>
Run rustfmt to clean up formatting of the code.

Signed-off-by: David Brown <[email protected]>
@d3zd3z d3zd3z requested review from teburd and cfriedt March 7, 2025 19:36
With Mutex having a const constructor, simplify the initializaion, and
use a simple static for the Stats container.

Signed-off-by: David Brown <[email protected]>
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I think it's a bit simpler with fewer clones and unwraps.

Users can also adjust the git ref that the Zephyr 4.1.0 release points to, if they want to try this out with the latest Zephyr release after it's merged.

@teburd
Copy link

teburd commented Mar 12, 2025

I wouldn't be super scared of Pin, but really most IPCs in Zephyr really do want to be static I'd think. Otherwise there's going to be lots of issues trying to track lifetimes of what lives longer than what else. Particularly because Zephyr loves callbacks and there's not a great way I don't think to inform Rust something must live longer than the callback. I could be wrong though!

@d3zd3z
Copy link
Collaborator Author

d3zd3z commented Mar 14, 2025

I wouldn't be super scared of Pin, but really most IPCs in Zephyr really do want to be static I'd think. Otherwise there's going to be lots of issues trying to track lifetimes of what lives longer than what else. Particularly because Zephyr loves callbacks and there's not a great way I don't think to inform Rust something must live longer than the callback. I could be wrong though!

I think it is quite reasonable to have various Zephyr APIs that use things like callbacks and such to accept &'static .... This change should be perfectly compatible with that, and in fact, for most of the types, now allows for plain static declarations, without even needing macros to support it.

@d3zd3z d3zd3z merged commit a341bc5 into main Mar 14, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants